-
Notifications
You must be signed in to change notification settings - Fork 13
Dynamic user agent #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces optional userAgent support in MailtrapClientConfig and updates MailtrapClient to set the User-Agent header accordingly. Adds and exposes Suppressions API via the client. Adjusts send flow to encode the payload prior to POST, with minor ordering around testInboxId validation unchanged functionally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Client as MailtrapClient
participant HTTP as Mailtrap API
Note over App,Client: Initialization
App->>Client: new MailtrapClient({ token, ... , userAgent? })
Client->>Client: Set headers (User-Agent: userAgent || default)
rect rgba(220,235,255,0.4)
Note right of Client: Send flow
App->>Client: send(mailPayload, options)
Client->>Client: validate testInboxId (if required)
Client->>Client: encode mailPayload
Client->>HTTP: POST /... with encoded payload + headers
HTTP-->>Client: Response
Client-->>App: Result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/config/index.ts (1)
21-21
: Option: include package version in UA for better telemetry.Consider appending the library version (e.g., "mailtrap-mcp/1.2.3 (...)"). Implement in get-agent.ts to keep config static.
src/lib/get-agent.ts (4)
23-30
: Make cwd-based detection case-insensitive.Paths can differ in case across platforms; normalize before matching.
Apply this diff:
function isWorkingDirectoryMCP(): boolean { try { const cwd = process.cwd(); - return cwd.includes("mailtrap-mcp") && !cwd.includes("node_modules"); + const lower = cwd.toLowerCase(); + return lower.includes("mailtrap-mcp") && !lower.includes("node_modules"); } catch { return false; } }
36-44
: Broaden self-exclusion and normalize case in stack-based detection.Also exclude
node_modules/mailtrap-nodejs
to avoid false positives when this lib is on the stack.Apply this diff:
function isCallStackMCP(): boolean { - const { stack } = new Error(); - - return !!( - stack && - stack.includes("mailtrap-mcp") && - !stack.includes("node_modules/mailtrap") - ); + const { stack } = new Error(); + const lower = stack?.toLowerCase(); + return !!( + lower && + lower.includes("mailtrap-mcp") && + !/node_modules\/mailtrap(-nodejs)?/.test(lower) + ); }
59-61
: Allow env override and ensure robust fallback.Let users force UA selection and guard against unexpected undefineds.
Apply this diff:
-function getDynamicUserAgent(): string { - return isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; -} +function getDynamicUserAgent(): string { + const override = process.env.MAILTRAP_UA?.toLowerCase(); + if (override === "mcp") return MCP_USER_AGENT; + if (override === "default") return USER_AGENT; + const ua = isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; + return ua || "mailtrap-nodejs"; +}
59-61
: Optional: add version to UA string.If
resolveJsonModule
is enabled, append package version for traceability (example implementation):// at top-level (outside selected range): // import { version } from "../../package.json"; // inside getDynamicUserAgent(): // const base = isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; // return `${base}/${version}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/config/index.ts
(1 hunks)src/lib/MailtrapClient.ts
(2 hunks)src/lib/get-agent.ts
(1 hunks)
🔇 Additional comments (4)
src/config/index.ts (1)
21-21
: LGTM: adds MCP-specific User-Agent.Field name and value look consistent with existing config keys.
src/lib/MailtrapClient.ts (2)
9-9
: LGTM: importing dynamic UA helper.
69-70
: LGTM: apply dynamic User-Agent header.Header is set once per client instance, which is appropriate.
src/lib/get-agent.ts (1)
1-63
: CommonJS-only packaging detected; no ESM support found.
package.json “type” is absent (defaults to CJS), tsconfig module=commonjs, and no ESM exports fields—no changes needed.
src/lib/get-agent.ts
Outdated
function isMainModuleMCP(): boolean { | ||
const mainFile = require?.main?.filename; | ||
|
||
return !!( | ||
mainFile && | ||
mainFile.includes("mailtrap-mcp") && | ||
!mainFile.includes("node_modules") | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix ESM ReferenceError risk from using require
directly.
require?.main
still throws in pure ESM because require
is an unbound identifier. Guard with typeof require !== 'undefined'
and fall back to process.argv[1]
.
Apply this diff:
function isMainModuleMCP(): boolean {
- const mainFile = require?.main?.filename;
-
- return !!(
- mainFile &&
- mainFile.includes("mailtrap-mcp") &&
- !mainFile.includes("node_modules")
- );
+ const mainFile =
+ (typeof require !== "undefined" ? require.main?.filename : undefined) ??
+ process.argv?.[1];
+ if (!mainFile) return false;
+ const lower = mainFile.toLowerCase();
+ return lower.includes("mailtrap-mcp") && !lower.includes("node_modules");
}
🤖 Prompt for AI Agents
In src/lib/get-agent.ts around lines 9 to 17, avoid referencing the unbound
require in pure ESM by first checking typeof require !== 'undefined' and using
require.main?.filename only when defined, otherwise fall back to
process.argv[1]; replace const mainFile = require?.main?.filename with a safe
resolution like: if typeof require !== 'undefined' and require.main use
require.main.filename else use process.argv[1], then keep the subsequent
includes checks the same to determine mailtrap-mcp presence and node_modules
exclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (12)
src/__tests__/lib/get-agent.test.ts (12)
41-41
: Use jest.spyOn instead of reassigning process.cwdReassigning globals is brittle and hard to restore. Spy on it instead.
Apply:
-process.cwd = jest.fn().mockReturnValue("/path/to/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/mailtrap-mcp");
54-56
: Use jest.spyOn for process.cwd here as wellRepeat the spy pattern.
Apply:
-process.cwd = jest - .fn() - .mockReturnValue("/path/to/node_modules/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue( + "/path/to/node_modules/mailtrap-mcp" +);
68-70
: Mock cwd error via spy, not reassignmentThis keeps the original method intact.
Apply:
-process.cwd = jest.fn().mockImplementation(() => { - throw new Error("Permission denied"); -}); +jest.spyOn(process, "cwd").mockImplementation(() => { + throw new Error("Permission denied"); +});
91-91
: Same here: prefer a spy for process.cwdApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
112-112
: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
133-133
: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
149-149
: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
162-165
: Combine both mocks using spies and real ErrorAlso prefer returning a real Error with overridden stack to preserve instanceof and other semantics.
Apply:
-process.cwd = jest.fn().mockImplementation(() => { - throw new Error("Permission denied"); -}); -global.Error = jest.fn().mockImplementation(() => ({ - stack: undefined, -})) as any; +jest.spyOn(process, "cwd").mockImplementation(() => { + throw new Error("Permission denied"); +}); +global.Error = jest.fn().mockImplementation(() => { + const e = new originalError("Test error"); + Object.defineProperty(e, "stack", { value: undefined }); + return e; +}) as any;Tip: apply the “real Error with overridden stack” pattern to other stack-based tests, too (Lines 82-89, 102-109, 124-131, 145-147).
181-183
: Prefer spy over direct assignmentApply:
-process.cwd = jest - .fn() - .mockReturnValue("/Users/user/projects/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue( + "/Users/user/projects/mailtrap-mcp" +);
195-195
: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/Users/user/projects/my-app"); +jest.spyOn(process, "cwd").mockReturnValue("/Users/user/projects/my-app");
82-89
: Return a real Error instance with a mocked stackReturning a plain object breaks Error semantics. Use the original constructor and override stack.
Apply:
-global.Error = jest.fn().mockImplementation(() => ({ - stack: ` -Error: Test error - at Object.<anonymous> (/path/to/mailtrap-mcp/index.js:10:1) - at Module._compile (internal/modules/cjs/loader.js:1063:30) - `, -})) as any; +global.Error = jest.fn().mockImplementation(() => { + const e = new originalError("Test error"); + Object.defineProperty(e, "stack", { + value: ` +Error: Test error + at Object.<anonymous> (/path/to/mailtrap-mcp/index.js:10:1) + at Module._compile (internal/modules/cjs/loader.js:1063:30) + `, + }); + return e; +}) as any;
206-256
: Strengthen the integration test: assert getDynamicUserAgent is usedRight now it only checks client is created. Mock get-agent and assert it’s called during client construction. Also align the config mock to default-export shape.
Apply:
- jest.doMock("../../config", () => ({ - CLIENT_SETTINGS: { + jest.doMock("../../config", () => ({ + __esModule: true, + default: { + CLIENT_SETTINGS: { SENDING_ENDPOINT: "https://send.api.mailtrap.io", BULK_ENDPOINT: "https://bulk.api.mailtrap.io", TESTING_ENDPOINT: "https://sandbox.api.mailtrap.io", GENERAL_ENDPOINT: "https://mailtrap.io", USER_AGENT: "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", MAX_REDIRECTS: 0, TIMEOUT: 10000, - }, - ERRORS: { + }, + ERRORS: { FILENAME_REQUIRED: "Filename is required.", CONTENT_REQUIRED: "Content is required.", SUBJECT_REQUIRED: "Subject is required.", FROM_REQUIRED: "From is required.", SENDING_FAILED: "Sending failed.", NO_DATA_ERROR: "No Data.", TEST_INBOX_ID_MISSING: "testInboxId is missing, testing API will not work.", ACCOUNT_ID_MISSING: "accountId is missing, some features of testing API may not work properly.", BULK_SANDBOX_INCOMPATIBLE: "Bulk mode is not applicable for sandbox API.", - }, - TRANSPORT_SETTINGS: { - NAME: "MailtrapTransport", - }, + }, + TRANSPORT_SETTINGS: { + NAME: "MailtrapTransport", + }, + }, })); + const getAgentMock = jest.fn(() => "ua-from-test"); + jest.doMock("../../lib/get-agent", () => ({ + __esModule: true, + default: getAgentMock, + })); + // Clear module cache and re-import jest.resetModules(); const { default: MailtrapClient } = jest.requireActual( "../../lib/MailtrapClient" ); @@ - // The User-Agent should be set in the axios instance - // We can't easily test the internal axios instance, but we can verify - // that the function is called during client creation - expect(client).toBeDefined(); + // Ensure dynamic UA is consulted during client creation + expect(client).toBeDefined(); + expect(getAgentMock).toHaveBeenCalledTimes(1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/lib/get-agent.test.ts
(1 hunks)src/lib/MailtrapClient.ts
(3 hunks)src/lib/get-agent.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/get-agent.ts
- src/lib/MailtrapClient.ts
🔇 Additional comments (1)
src/__tests__/lib/get-agent.test.ts (1)
29-39
: Good baseline coverage and scenariosNice coverage of defaults, cwd-based, call stack-based, and error edge cases.
src/__tests__/lib/get-agent.test.ts
Outdated
jest.mock("../../config", () => ({ | ||
CLIENT_SETTINGS: { | ||
USER_AGENT: | ||
"mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | ||
MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | ||
}, | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mock the default export shape of config to avoid interop breakage
The test imports CONFIG as a default export, but the mock returns a plain object. Under ts-jest/Babel interop, default may be undefined. Return { __esModule: true, default: ... }.
Apply this diff:
-jest.mock("../../config", () => ({
- CLIENT_SETTINGS: {
- USER_AGENT:
- "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)",
- MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)",
- },
-}));
+jest.mock("../../config", () => ({
+ __esModule: true,
+ default: {
+ CLIENT_SETTINGS: {
+ USER_AGENT:
+ "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)",
+ MCP_USER_AGENT:
+ "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)",
+ },
+ },
+}));
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
jest.mock("../../config", () => ({ | |
CLIENT_SETTINGS: { | |
USER_AGENT: | |
"mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | |
MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | |
}, | |
})); | |
jest.mock("../../config", () => ({ | |
__esModule: true, | |
default: { | |
CLIENT_SETTINGS: { | |
USER_AGENT: | |
"mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | |
MCP_USER_AGENT: | |
"mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | |
}, | |
}, | |
})); |
🤖 Prompt for AI Agents
In src/__tests__/lib/get-agent.test.ts around lines 4 to 10, the jest.mock
currently returns a plain object which breaks default-import interop under
ts-jest/Babel; update the mock to export a proper ES module shape by returning
an object with __esModule: true and a default property containing the original
CLIENT_SETTINGS object so the imported CONFIG default is defined.
src/__tests__/lib/get-agent.test.ts
Outdated
let originalCwd: string; | ||
let originalError: typeof Error; | ||
|
||
beforeEach(() => { | ||
// Store original values | ||
originalCwd = process.cwd(); | ||
originalError = global.Error; | ||
}); | ||
|
||
afterEach(() => { | ||
// Restore original values | ||
process.cwd = jest.fn().mockReturnValue(originalCwd); | ||
global.Error = originalError; | ||
jest.clearAllMocks(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore the real process.cwd; don’t replace it with a mock
You’re saving the cwd value (string) and then replacing process.cwd with a mock that returns that string. This leaks a mocked function into global state across the suite.
Apply this minimal fix (and prefer spy-based mocking in tests):
- let originalCwd: string;
+ let originalCwdFn: typeof process.cwd;
let originalError: typeof Error;
beforeEach(() => {
// Store original values
- originalCwd = process.cwd();
+ originalCwdFn = process.cwd;
originalError = global.Error;
});
afterEach(() => {
// Restore original values
- process.cwd = jest.fn().mockReturnValue(originalCwd);
+ process.cwd = originalCwdFn;
global.Error = originalError;
- jest.clearAllMocks();
+ jest.restoreAllMocks();
});
Follow-up (recommended): replace all direct assignments to process.cwd in tests with jest.spyOn(process, "cwd").mockReturnValue(...).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let originalCwd: string; | |
let originalError: typeof Error; | |
beforeEach(() => { | |
// Store original values | |
originalCwd = process.cwd(); | |
originalError = global.Error; | |
}); | |
afterEach(() => { | |
// Restore original values | |
process.cwd = jest.fn().mockReturnValue(originalCwd); | |
global.Error = originalError; | |
jest.clearAllMocks(); | |
}); | |
let originalCwdFn: typeof process.cwd; | |
let originalError: typeof Error; | |
beforeEach(() => { | |
// Store original values | |
originalCwdFn = process.cwd; | |
originalError = global.Error; | |
}); | |
afterEach(() => { | |
// Restore original values | |
process.cwd = originalCwdFn; | |
global.Error = originalError; | |
jest.restoreAllMocks(); | |
}); |
🤖 Prompt for AI Agents
In src/__tests__/lib/get-agent.test.ts around lines 13-27, the test saves only
the cwd string and then replaces process.cwd with a mock that returns that
string, leaking a mocked function into global state; change the setup to save
the original process.cwd function (e.g., originalCwdFn = process.cwd) and in
afterEach restore the original function (process.cwd = originalCwdFn) instead of
assigning a jest.fn(), and, going forward, use jest.spyOn(process,
"cwd").mockReturnValue(...) when mocking cwd in tests so the spy can be restored
automatically.
src/lib/get-agent.ts
Outdated
* Gets the appropriate User-Agent string based on the current context. | ||
* @returns The User-Agent string for the current context | ||
*/ | ||
function getDynamicUserAgent(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you ask me, bringing the MCP concern to the SDK project is a violation of the SRP 🙂 I would approach this differently and allow specifying the user agent directly via a configuration setting of the SDK client. And, override this setting in the MCP server NPM package.
It's simpler, and it's not a big deal to allow users of the SDK to override the user agent; it's fairly easy to do this anyway.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if allowing users to override user agent is not a problem, then its simple solution. I thought that we don't want to enable users to change the user agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanchuk WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @vittorius, it's cleaner to allow to override user agent, and it might be useful for other purposes (for the users), too.
…USER_AGENT constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/MailtrapClient.ts
(4 hunks)src/types/mailtrap.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/MailtrapClient.ts (1)
src/types/mailtrap.ts (1)
MailtrapClientConfig
(64-71)
🔇 Additional comments (5)
src/lib/MailtrapClient.ts (5)
15-15
: Suppressions API addition not mentioned in PR objectives.The import of
SuppressionsBaseAPI
appears unrelated to the PR's stated purpose of adding dynamic User-Agent detection. This suggests either:
- The PR description is incomplete, or
- Unrelated changes were bundled into this PR
Consider splitting unrelated changes into separate PRs for clearer review and git history.
62-63
: User-Agent parameter implementation looks correct.The constructor correctly accepts the optional
userAgent
parameter matching the type definition.
70-70
: Static User-Agent configuration instead of dynamic detection.This implementation uses a static fallback pattern (
userAgent || USER_AGENT
) rather than the dynamic MCP context detection described in the PR objectives. The PR objectives mention:
- MCP context detection using main module filename
- Working directory analysis
- Call stack analysis
- MCP runtime detection
None of this detection logic is present here. The user must manually pass the correct
userAgent
value.If the detection logic exists in a separate file (e.g., a
get-agent.ts
utility), it should be called here instead of the simple fallback. Something like:"User-Agent": userAgent || getAgent() || USER_AGENT,Where
getAgent()
would perform the MCP detection logic described in the PR objectives.
154-158
: Suppressions API getter not related to PR objectives.The addition of the
suppressions
getter provides access to the Suppressions API, but this is not mentioned in the PR objectives which focus solely on dynamic User-Agent detection.Consider moving this to a separate PR focused on API additions for better change tracking and clearer git history.
191-192
: Minor refactor: Extract encoded mail to variable.Extracting
encodeMailBuffers(mail)
to apreparedMail
variable improves readability without changing functionality.
Motivation
The Mailtrap Node.js SDK needed to automatically detect when it's running within a Mailtrap MCP (Model Context Protocol) environment and use an appropriate User-Agent string. This improves environment detection, integration tracking, and provides better analytics for MCP-specific usage patterns.
Changes
mailtrap-nodejs
andmailtrap-mcp
based on runtime contextMCP_USER_AGENT
configuration option to support MCP-specific User-Agent stringMailtrapClient
to use dynamic User-Agent selection instead of static configurationHow to test
npm test -- src/__tests__/lib/get-agent.test.ts
to verify all 12 unit tests passmailtrap-nodejs
User-Agentmailtrap-mcp
User-Agentprocess.cwd()
throws exceptionsSummary by CodeRabbit
New Features
Improvements